Adding a sample Firebolt plugin. #784
Conversation
| @@ -0,0 +1,6 @@ | |||
| startmode = "@PLUGIN_FIREBOLTPRIVACY_AUTOSTART@" | |||
| callsign = "Privacy" | |||
There was a problem hiding this comment.
Is this intentional? Typically we use the name of the file (FireboltPrivacy -> FireboltPrivacy.json) and than the callsign automatically becomes FireboltPrivacy. You are now overruling this name by setting the callsign. This is, like mentioned deviating from the standard.... If required, please make sure to document it her that it is required for backwards compatibility.
There was a problem hiding this comment.
Yes, this is intentional. The Firebolt APIs didn't have the prefix Firebolt (eg. Privacy.AllowResumePoints) . I have changed the callsign to Privacy to maintain the same method names. I felt having Privacy alone as the plugin name, doesn't add meaning hence kept the plugin name as FireboltPrivacy and callsign as Privacy.
| _fireboltPrivacy = service->Root<Exchange::IFireboltPrivacy>(_connectionId, RPC::CommunicationTimeOut, _T("FireboltPrivacyImplementation")); | ||
| if (_fireboltPrivacy != nullptr) { | ||
| _fireboltPrivacy->Register(&_connectionNotification); | ||
| Exchange::JFireboltPrivacy::Register(*this, _fireboltPrivacy); |
There was a problem hiding this comment.
Why not do this as the final step? Only hookup the JSONRPC interface if all the other steps succeeded..
| _service->Unregister(&_connectionNotification); | ||
|
|
||
| if (_fireboltPrivacy != nullptr) { | ||
| Exchange::JFireboltPrivacy::Unregister(*this); |
There was a problem hiding this comment.
Make sure if you register the JSONRPC only if all succeeded, that you only Unregister it if all was succeeded..
| #include <interfaces/IConfiguration.h> | ||
| #include <interfaces/IFireboltPrivacy.h> | ||
| #include <interfaces/IDictionary.h> | ||
| #include <interfaces/IStore.h> |
There was a problem hiding this comment.
IStore.h and IDictionary.h are different implemtations for the sme functionality. Suggest to focus on IDictionary than and not IStore. (so only include one of them, preferably IDictionary.h)
Going to the remainder of the code. I see you already use the IDictionary, which is good, so remove the reference to IStore.h.
| Core::hresult Register(Exchange::IFireboltPrivacy::INotification* notification) override | ||
| { | ||
| ASSERT(notification != nullptr); | ||
| ASSERT(std::find(_notifications.begin(), _notifications.end(), notification) == _notifications.end()); |
There was a problem hiding this comment.
Move the assert within the Lock.
| ASSERT(notification != nullptr); | ||
| ASSERT(std::find(_notifications.begin(), _notifications.end(), notification) == _notifications.end()); | ||
| _adminLock.Lock(); | ||
| notification->AddRef(); |
There was a problem hiding this comment.
There is no need to put the AddRef within the lock.
| ASSERT(std::find(_notifications.begin(), _notifications.end(), notification) == _notifications.end()); | ||
| _adminLock.Lock(); | ||
| notification->AddRef(); | ||
| _notifications.push_back(notification); |
There was a problem hiding this comment.
As a fool-proof example (people will copy-paste this :-) ) only add the Notification interface if it is not find. Still ASSERT on it, but in Release we will not add it and return an error code for it)
| } | ||
| _adminLock.Unlock(); | ||
|
|
||
| return Core::ERROR_NONE; |
There was a problem hiding this comment.
Please report an error if you could not unregister it, also ASSERT on it if the notification could not be found.
We promote defensive coding these ASSERTS help in these defensive coding.
| _adminLock.Unlock(); | ||
| } else { | ||
| if (_localStore == nullptr) { | ||
| _localStore = _service->QueryInterfaceByCallsign<Exchange::IDictionary>("Dictionary"); |
There was a problem hiding this comment.
I do not think that magicals in the code show proper coding ;-) Why not have 1 config value, specified as "Dictionary" default callsign for the Dictionary plugin. Than if you want to use in memory, in config, specify this "callsign": null that we want to take it out of memory. This way, if the dictionary plugin is called org.rdk.dictionary, you do not need to change a single line of code.
| allowResumePoints = _allowResumePoints; | ||
| _adminLock.Unlock(); | ||
| } else { | ||
| if (_localStore == nullptr) { |
There was a problem hiding this comment.
There are several serious flaws here that must be resolved properly!
Use case scenario 1 (race condition):
First run, _localStore == nullptr, the Thread gets pre-empted after the if.
Another thread also executes this method. it will find _localStore == nullptr...
You do the math..
Use case scenario 2 (functional flaw):
- _localStore == nullptr, you grap the first time the IDictionary of the plugin..
- The dictionary plugin is deactivated.
- Again a call is made to this method... you do the math..
Solution: https://github.com/rdkcentral/Thunder/blob/master/Source/plugins/Types.h#L239C11-L239C30 us this class and all should be peachy..
| } | ||
| if (_localStore != nullptr) { | ||
| string value; | ||
| _localStore->Get("FireboltPrivacy", "AllowResumePoints", value); |
There was a problem hiding this comment.
How about a:
static TCHAR* constexp PrivacyNamespace[] = _T("FireboltPrivacy");
static TCHAR* constexp PrivacyKeyAllowResume[] = _T("AllowResumePoints");
Bottomline, magicals should be located in 1 place preferably in a header, so we can all use them and not all use a text literal.
| if (_localStore != nullptr) { | ||
| string value; | ||
| _localStore->Get("FireboltPrivacy", "AllowResumePoints", value); | ||
| if (value.empty() || value == "false" ) { |
There was a problem hiding this comment.
False is not allowed ? Or FALSE ?
| } | ||
| } else { | ||
| SYSLOG(Logging::Error,(_T("UNABLE TO GET DICTIONARY INTERFACE")) ); | ||
| return Core::ERROR_RPC_CALL_FAILED; |
There was a problem hiding this comment.
See coding guidelines and I think it is even a rule, one return per method (Maintainability)
| } | ||
| _adminLock.Unlock(); | ||
| } else { | ||
| if (_localStore == nullptr) { |
There was a problem hiding this comment.
Same issues as in the get :-)
| _localStore->Set("FireboltPrivacy", "AllowResumePoints", "false"); | ||
| } | ||
| _adminLock.Lock(); | ||
| for(auto notification: _notifications){ |
There was a problem hiding this comment.
Design flaw:
This sounds as a bad idea... The JSONRPC is living in the WPEFramework process. In case we are running this out-of-process, it means that for each subscriber we need to pass the process boundary (expensive). Move the JSONRPC notification handling into the Plugin itself and report once the notification from here and the broadcast from the event from the WPEFramework process.
Given the usecase that one might be waiting on this information and than callback into this plugin, you have a deadlock. Suggest to decouple this notification in WPEFramework as there you also have a Workerpool AND you migh accumulate pending updates.
| Config config; | ||
| config.FromString(service->ConfigLine()); | ||
| _service = service; | ||
| Core::JSON::EnumType<Exchange::IFireboltPrivacy::StorageLocation> storageLocation = config.StorageLocation.Value(); |
There was a problem hiding this comment.
As suggested, oly use a callsign to odentify a plugin that holds the IDictionary callsign, if null is set on this value assume in memory..
| if (storageLocation == Exchange::IFireboltPrivacy::StorageLocation::InMemory) { | ||
| _inMemory = true; | ||
|
|
||
| _localStore = _service->QueryInterfaceByCallsign<Exchange::IDictionary>("Dictionary"); |
There was a problem hiding this comment.
I think the https://github.com/rdkcentral/Thunder/blob/master/Source/plugins/Types.h#L239C11-L239C30 is the answer to all your issues....
|
|
||
| private: | ||
| PluginHost::IShell* _service; | ||
| mutable Core::CriticalSection _adminLock; |
There was a problem hiding this comment.
Mutable is only needed if you want to call non-const methods on a member object (like Core:CriticalSection) in a const method in this class. Right now you do not need it as there are no const methods in this class the require the access to the _adminLock. However after resolving all the review remarks, you do need it ;-)
| #define MODULE_NAME Plugin_FireboltPrivacy | ||
| #endif | ||
|
|
||
| #include <core/core.h> |
There was a problem hiding this comment.
If you include the core/core.h here as being a dependency also put it in the CMakeLists.txt (consistency)
|
|
||
| #include <core/core.h> | ||
| #include <plugins/plugins.h> | ||
| #include <messaging/messaging.h> |
There was a problem hiding this comment.
See core (also applicable than to messaging :-)
There was a problem hiding this comment.
Make sure that the modules you include here, end up here: https://github.com/rdkcentral/Thunder/blob/master/Source/plugins/Types.h#L239C11-L239C30
| target_link_libraries(${MODULE_NAME} | ||
| PRIVATE | ||
| CompileSettingsDebug::CompileSettingsDebug | ||
| ${NAMESPACE}Plugins::${NAMESPACE}Plugins |
There was a problem hiding this comment.
Finding the package required is step 1, inclusing them is step 2 :-) You should include Core and Messaging here as well.
WARNING: A Blackduck scan failure has been waivedA prior failure has been upvoted
|
No description provided.